fix: cap the connections a web listen serves at once - #693
Merged
Conversation
listen() and listen_tls() spawned a task per accepted connection with no ceiling. a client opening connections faster than they complete grew the task count without bound, and every green task carries a pooled stack, so that is memory growth rather than scheduling pressure. every other accept loop in std already bounds itself. at the cap these loops refuse rather than block, which is the opposite of what the http/2 loops do, because the same mechanism has a different consequence on the two protocols. an http/2 connection is a multiplexer: requests arrive on connections that are already open, so a loop that stops accepting keeps serving at full rate, health check included. http/1.1 has no such decoupling — a request needs a connection — so a loop that stops accepting stops answering everything, and the process is killed for being unresponsive at the moment it is healthy and full. a refused connection costs one write and one close on the accept loop and never a task, so the memory the cap protects is still protected. the slot is claimed on the accept loop next to the drain count and given back by the same `defer` on the connection task, ordered so it lands before the drain count: a drain that reports zero has already returned every slot. two new series make the cap visible on a scrape, recorded by the accept loops rather than the request middleware so a bare() app has them too: http_server_connections pinned at the cap with no traffic is a stalled-connection wedge, and http_server_connections_refused_total climbing is the cap shedding load. nothing exposed either for the http/2 loops, which still have none.
docs/web.md gets a section under "a task per connection" covering the 512- connection budget, why these loops refuse where the http/2 loops block, and the two new series. it is honest about the 503 being best-effort: closing a socket that still holds unread data resets it, and the peer that had already sent its request — most of them, when a cap is firing — loses the status line. draining first would need a read with no deadline under green, which would park the accept loop for good, so the counter is the signal and the 503 is a courtesy. docs/concurrency.md now says when to reach for a semaphore and when for a counter in a compare_set loop: waiting is the whole of what a semaphore adds, and there is no non-blocking way to take a permit. the new tests move to ports the h2c tests do not already use.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
web.listenspawned a task per accepted connection with no ceiling. a clientthat opens connections faster than they complete grows the task count without
bound, and every green task carries a pooled stack, so that is memory growth
rather than scheduling pressure.
listen_tlshad the same gap — it runs its ownaccept loop rather than delegating, so both plaintext and tls were uncapped.
listen_h2cwas the odd one out: it delegates tostd.net.http2.serverandinherited that module's cap, which meant one of the three web entry points was
bounded and two were not, with nothing in the api to say which.
refusing rather than blocking
the four http/2 accept loops block on a permit before spawning. that is the wrong
answer here, and the reason is the protocol rather than a preference.
an http/2 connection is a multiplexer. requests arrive on connections that are
already open, so a loop that stops accepting keeps serving requests at full rate
— including a health check on any established connection. blocking costs new
peers their connection and nothing else, which is what backpressure should cost.
http/1.1 has no such decoupling. a request needs a connection, and past the first
keep-alive burst a new request needs a new accept. a loop that stops accepting
stops answering everything: the load balancer's probe, the orchestrator's
liveness check,
GET /healthz. the process is then killed for beingunresponsive at exactly the moment it is healthy and full, and the replacement
comes up into the same load. worse, the failure reports itself inconsistently —
a tcp-level probe still succeeds against a backlogged listener, so whether the
server looks alive depends on how it is measured.
so these loops accept and refuse:
503 Service UnavailablewithRetry-After,written inline on the accept loop, no task spawned. the memory the cap exists to
protect is still protected, because refusing costs one write and one close. over
tls the socket is closed instead — saying 503 in http would mean completing the
key exchange first, which is the expensive work being refused.
the 503 itself is best-effort, and the code says so rather than implying
otherwise. closing a socket that still holds unread data sends RST instead of
FIN, and a client that gets RST discards what it had buffered — so a peer that
had already sent its request, which is most of them at the moment a cap fires,
sees a connection error rather than the status line. draining the request first
is the fix and it is not available: a read on a socket whose peer sent nothing
has no deadline under the green runtime and would park the accept loop forever,
which is the failure the whole design is avoiding. the response can only improve
on the reset, never replace it, so the operator's signal is the counter and the
503 is a courtesy to the peers that happen to be waiting. the test exercises the
clean-FIN case and says which one it is.
the shape changes with the decision.
connection_slotsis aSemaphore, and asemaphore's whole contribution over a counter is that
acquire()waits; there isno non-blocking way to take a permit. remove the waiting and what is left is a
counter, so this is an
AtomicIntclaimed withcompare_setin a retry loop.the claim still sits where #691 put the drain count — on the accept loop, before
the
spawn— and is given back by the samedeferon the connection task,ordered to land before
shutdown.leave()so a drain that reports zero hasalready returned every slot.
against inert timeouts
tcp_set_timeoutdoes nothing under the green runtime (the green read path waitson the reactor with no deadline), so
CONNECTION_TIMEOUT_MSdoes not reclaim theslot of a connection that stalls. that is a separate bug and is not touched here,
but it decides how this design has to degrade. under a blocking cap a set of
stalled connections ends in a silent wedge: no accepts, no answers, nothing to
look at. refusing turns the same situation into a server that answers every
connection immediately, with a refusal counter climbing and a live-connection
gauge pinned at the cap — which is that wedge's exact signature, on a scrape.
observability
nothing in
std.metricscovered the http/2 loops, so there was nothing to match.two series are added, recorded by the accept loop rather than the request
middleware so a
web.bare()app has them too:http_server_connections— a gauge of connections being servedhttp_server_connections_refused_total— a counter of connections refusedthe http/2 loops still expose neither.
what was tested
three colocated tests in
std/web.pith, each run underPITH_GREEN=1andPITH_GREEN=0. they drivespawn_conn/spawn_tls_conndirectly over realsocket pairs, the way the drain tests added in #691 do, so nothing waits for a
listener to bind and there is no sleep anywhere in them. holding 512 real sockets
open would cost more descriptors than a test should, so the counter is filled to
one below the cap through the same claim the accept loop uses and the two
connections that follow straddle it exactly.
connections: the first is admitted and appears in the drain count, the second
is refused. the hand-off returning at all is the assertion — under a blocking
cap the test never reaches the next line. the refused peer reads back a
complete
503 Service Unavailablewithout having sent a request, the refusalcounter moves by exactly one, and no second task or drain entry appears. then
the held slots are dropped and a fresh connection is admitted and served
pong, so the cap sheds load rather than latching.spawn_conn, counter exact at +6the instant the last hand-off returns, each driven to a real
pong, thendrain(5000) == 0and the counter back to its baseline. this is the leak case— a slot kept here shrinks the cap on every burst until the server refuses
everything, hours later, with nothing to point at.
handshake has read a byte, and gives it back when the handshake fails after its
peer disappears — the failure path, which is the one a hostile client picks.
past the cap the socket is closed with no task, no drain count, and no
handshake, and the peer reads end-of-stream rather than a server hello.
each was falsified against the code it claims to cover, on the default backend:
MAX_CONNECTIONS + 1002 != 1,1 != 0)release_connection_slotfromhandle_conn1 != 0,6 != 0)release_connection_slotfromhandle_tls_socket1 != 0)that last one is the design decision demonstrated: with the loop parked on a
slot, the test process stops answering entirely, which is what a blocked accept
loop does to a health check.
make run-regressions-onlyat 332/332,make docsite-checkclean.